feat(workspace): route warehouse tools through the bound workspace's engine - #1168
feat(workspace): route warehouse tools through the bound workspace's engine#1168ralphstodomingo wants to merge 2 commits into
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 34260894 | Triggered | Generic CLI Secret | 83c5075 | packages/opencode/test/cli/help/snapshots/help-snapshots.test.ts.snap | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
Review logStacked on #1167 — its review-log comment carries the attach contract (claims, disclosed residuals, review policy). This PR is reviewed the same way: a finding is a reproducible trace that violates a claim; rounds are capped. Claims (in addition to #1167's)
Disclosed residuals (in addition to #1167's)
End-to-end rows (from #1156, re-run on this stack 2026-08-28 against the demo workspace with a Snowflake connection; engine 0.7.0)
Rounds(none yet) Codex rounds
CI note — GitGuardian is red on this PR and that is a false positive. The "1 secret" is the literal placeholder |
|
@codex review against the numbered claims and the disclosed residuals in the review-log comment on this PR: report only a reproducible trace that violates a numbered claim; an instance of a listed residual is disclosed behaviour, not a finding. |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
edf9c44 to
b2e5c6a
Compare
…engine Shadow a native warehouse capability only when the bound workspace's engine materialised the matching tool and attach attests the engine is its own (outcome `attached` plus the configured pin); redirect to the exact engine tool after the native safety checks; fail open with a reason otherwise. `--integrations=local` turns it off. Restacked onto the derived-overlay attach; the allowlist is exactly `attached`.
…ormat tools.ts The two `describeNativeTool` call sites used the single-line marker form, which the strict marker guard that runs on pushes to main does not recognise. No behaviour change.
893ff8f to
b8daef2
Compare
There was a problem hiding this comment.
Consensus review from an 8-model panel (Claude + GPT 5.4 Codex, Gemini 3.1 Pro, Kimi K2.5, MiniMax M2.7, GLM-5.1, Qwen 3.6), two convergence rounds. Reviewed against the numbered claims and disclosed residuals in the review-log comment; instances of a listed residual are not reported.
No blockers. 6 major, 6 minor, 3 nit. The major items are inline below; minor and nit items are in a separate comment.
Fix first: MAJOR #1 — when the engine can't be attributed, the call runs locally with no notice and no undetermined marker in the result. The only channel is a TUI toast, so in headless there is no signal at all. It fires on every affected session rather than under a race, and it means the pilot's own telemetry can't distinguish "routed" from "quietly didn't". A few lines in check().
The remaining majors read as reasonable pilot residuals. Note that MAJOR #2's inherited half (a mutable registry each handler re-resolves independently) and MINOR M0's inherited half (the attached outcome carrying no workspace identity, in engine-types.ts) both live below this PR in the stack.
The design itself held up well under seven independent reads — capability-scoped shadowing, the reachable() gating, canonicalType inverting DRIVER_MAP, and the announcement machinery were all singled out as correct, and the guard-ordering test genuinely proves its invariant rather than asserting it. Every finding here is about a seam, not the shape of the decision.
| try { | ||
| const directory = Instance.directory | ||
| if (!directory) return null | ||
| const binding = await readLocalBinding(directory) |
There was a problem hiding this comment.
MINOR — workspace identity drops the credential scope, so a redirect can cross tenants
Severity note: raised by the panel as a blocker on the strength of "cross-tenant". Recording it as MINOR — each customer occupies a single tenant, so no customer can reach this. The only actor is an internal staff session switching tenants mid-flight, inside a roughly one-turn window. Real defect, narrow and internally-bounded trigger, cheap fix.
readLocalBinding is readLocalBindingScoped(...).binding — it discards the scope. The subsystem this sits on top of deliberately does not, and says why:
// engine-overlay.ts:151-156
/** Identity of the workspace a binding names: the credential scope it was
* read under plus the tenant-local id. */
function workspaceKey(binding: ScopedBinding): string {
return `${binding.scope ?? ""}|${binding.datamateId}`
}state.ts:195-205 exists purely to carry that scope (tenant|apiUrl): "Workspace ids are tenant-local; the scope is what tells the same id in two tenants apart."
Trace: a session attaches under tenant A / workspace 42. The user switches credential scope and links tenant B / workspace 42 before the next engine boundary. Then:
attested()passes — the settled outcome is{ kind: "attached"; available; declared?; missing? }(engine-types.ts:26) and carries no workspace identity at all.attributedTo()passes — the configured pin is--datamate 42, scope-free by construction.- the re-link guard at
precedence.ts:565passes — it compares42 !== Number("42").
Precedence engages and issues a redirect naming the engine tool that the pinned MCP wrapper still points at, which is tenant A's engine — so the query runs on the wrong credentials and is audited against the wrong workspace. Low-numbered id collisions across tenants (a demo workspace 1, a customer workspace 1) are ordinary.
It is silent when it happens: it would surface as a query in a customer's audit log that nobody on their side ran, not as anything visible in telemetry.
Fix — thread the scope through, as the overlay already does:
- read via
readLocalBindingScopedhere; - store
workspaceKey = \${scope}|${datamateId}`inPrecedenceinstead ofworkspaceId(precedence.ts:419,:565`); - carry the applied workspace key on the settled
attachedoutcome, and require settled identity, current binding and snapshot to match exactly; - when the scope is unavailable, run locally with an
undeterminednotice.
Note on ownership: the fix splits across the stack. Using the scoped reader is this PR. Putting identity on the attached outcome is engine-types.ts, i.e. #1167 — untouched here. Worth deciding which PR carries which half, or it falls between the two.
| precedence: "undetermined", | ||
| } | ||
| } | ||
| if (!precedence.enabled) return RUN |
There was a problem hiding this comment.
MAJOR — unattributed runs locally with no notice and no marker
if (!precedence.enabled) return RUN fires for every disabled reason, unattributed included. RUN is the empty verdict: no notice, no precedence: "undetermined" metadata. The only place the reason surfaces is a fire-and-forget TuiEvent.ToastShow.
In headless run/serve, when the event bridge fails, or simply before the toast lands, a session whose engine could not be attributed executes locally, skips the server-side audit, and says nothing in the result. That contradicts the module's own opening principle ("anything undetermined runs locally with an explicit notice; nothing is ever silent") and Claim 1 ("any other outcome, or none, fails open with a stated reason"). The toast is UI, not the correctness mechanism.
The same asymmetry appears just below at :575-576: an explicitly named connection whose configured type will not canonicalise returns a bare RUN, while the default-target path reports exactly that condition with a notice.
Fix: return a notice with precedence: "undetermined" for unattributed, and for an unrecognisable named type. pilot-off, escape-hatch, unbound and nothing-materialised can stay bare RUN — those are deliberate disablement, not uncertainty.
| // computed against (a concurrent `warehouse.add` can change which name sorts first). | ||
| // Reading once here makes the decided connection and the executed connection the | ||
| // same by construction. The dbt-first ordering below is unchanged. | ||
| const fallbackName = params.warehouse || Registry.list().warehouses[0]?.name |
There was a problem hiding this comment.
MAJOR — time-of-check/time-of-use between the routing decision and the executed target
This pin, and the check at :497-501, close the window across the dbt await. But the routing decision was made earlier and elsewhere: Precedence.check() → resolveDefaultTarget (register.ts:139-160) does its own Registry.list().warehouses[0] read from inside the tool body, and the handler then resolves the target again, independently. The await Dispatcher.call(...) boundary and the handler's own awaits are enough for a queued concurrent mutation to land in between, so the comment's claim that this makes the decided and executed connection "the same by construction" is stronger than what the pin actually does.
Concretely:
- the guard sees an unserved DuckDB default; a concurrent
warehouse.removedrops it;sql.explainorschema.inspectthen picks the newly-first Snowflake connection and executes it locally, despite Snowflake being shadowed — unaudited execution on a served connection, the exact outcome this design exists to prevent; - for an explicit name, a concurrent
warehouse.addcan replace that name with a served type aftercheck()read it. The handler pins the already-replaced type and sees no subsequent change, so this check cannot detect that window.
Note also that this pin exists only in register("sql.execute") — sql.explain (:552-570) and schema.inspect (:678-691) have no equivalent guard at all.
Fix: make the decision and the target acquisition atomic — move the precedence check into the handler after it pins the target (passing sessionID through), or return a lease {name, canonicalType, generation} that handlers must revalidate. Apply it to all three ops, explicit names included.
Related, same seam: Precedence.check()'s await import("../native/connections/register") (precedence.ts:586-588) has no try/catch, and check() is called outside the surrounding try in all three tool bodies — so a throw there takes out sql_execute, sql_explain and schema_inspect together instead of failing open.
default-target.test.ts:123-151 does not prove its stated invariant: it calls the dispatcher directly, omitting the preceding precedence decision, which is where the race actually is.
| // when the cached answer is about to enable, and leave the refusing path cheap | ||
| // rather than re-reading all config on every turn. | ||
| if (cached !== expected) return cached | ||
| await Config.invalidate().catch((err) => { |
There was a problem hiding this comment.
MAJOR — Config.invalidate() flushes the global cache and every instance's cache, once per turn; and a failed invalidation is trusted
Two things in these twelve lines.
(a) Blast radius. Config.invalidate() runs invalidateGlobal and invalidateAllInstances() (config/config.ts:827-831, ScopedCache.invalidateAll) — the call-site comment reasons about a per-instance cache, but this is process-wide. It runs whenever the cached pin already matches, which for an engaged session is every turn (refresh is called per turn from prompt.ts:1763-1774). In a long-running multi-directory serve, one active session's precedence refresh invalidates configuration for every other project, repeatedly, and active directories can end up thrashing each other's caches.
(b) A failed invalidation is swallowed. The .catch() logs and continues, so the second read() returns the same cached value, it matches expected, and routing engages on a pin that may no longer be on disk. If an IDE rewrote the entry from workspace 42 to 99 between turns and the invalidation fails, this returns the stale "42". Everywhere else this module refuses when it cannot establish attribution; this is the one path that proceeds instead — and it is the direction the function's own comment calls dangerous.
Fix: (a) don't invalidate globally on this hot path — expose a current-instance-only invalidation, do a narrow uncached read of the datamate entry, or use the overlay's attested applied identity (which would also address the scope blocker). (b) return null when the invalidation throws.
| // is what keeps precedence correct when an engine's tool set changes under us. | ||
| // Resolved before the loops below because both sides' descriptions depend on it. | ||
| const mcpTools = await MCP.tools() | ||
| const precedence = await Precedence.refresh( |
There was a problem hiding this comment.
MAJOR — precedence ignores per-turn tool availability
refresh() is derived from the full materialised MCP map plus permission rules. But resolveTools in llm.ts:309-316 deletes any tool where input.user.tools?.[tool] === false, after precedence has been computed here.
So a request that disables datamate_snowflake_execute_database_query for the turn still gets sql_execute shadowed: the native description falsely claims redirection, and the redirect names a tool that is not in that turn's catalogue. A working local operation becomes a dead end.
reachable() was written to prevent exactly this class of dead end for permissions — the same reasoning applies to availability.
Fix: derive precedence from the effective catalogue after user.tools toggles are applied, or pass an availableToolKeys set into refresh() and require materialised and available. Keep the permission-rule check as a separate condition.
|
|
||
| function bindTo(id = 42, name = "analytics") { | ||
| precedenceInternals.binding = async () => ({ datamateId: id, datamateName: name }) | ||
| precedenceInternals.attributedTo = async () => String(id) |
There was a problem hiding this comment.
MAJOR (testing) — mechanism 1a, attribution, is never exercised
Every test in this suite and in precedence-guard-order.test.ts:47-51 assigns precedenceInternals.attributedTo, .binding and .attachOutcome. The real attributedTo — the Config.get() read, the pinnedWorkspace parse, and the "confirm against disk only when the cached answer is about to enable" logic — is never executed by anything.
That is where three of this review's findings live: the credential-scope blocker, the global Config.invalidate() on the hot path, and the swallowed invalidation that trusts a stale pin. None of them could have been caught by CI, because no test reaches the code. The 1000 lines here prove the decision tree is right; they prove nothing about its inputs.
This needs an integration-shaped test, not a unit test: drive attributedTo against a temp config with pinned / unpinned / re-pinned datamate entries, and add one for the same numeric id under two credential scopes.
Minor, same file: resetForTests() (precedence.ts:438-446) deletes announce, binding and attributedTo but not attachOutcome, so that seam leaks between test files.
Consensus review — minor, nit, and rejected findingsCompanion to the inline review (6 major, no blockers). Panel: Claude + GPT 5.4 Codex, Gemini 3.1 Pro, Kimi K2.5, MiniMax M2.7, GLM-5.1, Qwen 3.6 — two convergence rounds. Reviewed against the numbered claims and disclosed residuals; instances of a listed residual are not reported. MINORM0. Workspace identity drops the credential scope — Recorded as MINOR rather than the blocker the panel first ranked it. Each customer occupies a single tenant, so no customer can reach it; the only actor is an internal staff session switching tenants mid-flight, in a roughly one-turn window. Real defect, cheap fix, deliberately not scheduled — details and the ownership split with #1167 are in the inline comment. M1. It reads M2. Hardcoded engine-tool conventions fail silently — Both are hand-maintained. A new engine integration ( M3. The dbt-fallback redirect names the fallback connection and offers no way to insist on dbt — With a dbt project on DuckDB (known, unserved) and a served Postgres as the registry fallback, the call is redirected to M4. The M5. Two coverage gaps CI cannot see — The dbt-fallback tests are Separately, NIT
Additional missing testsBeyond those named inline:
(A previous version of this comment listed a followed Raised and rejectedRecorded so they are not raised again in a later round:
What holds upWorth saying, because seven independent reads converged on it: the design is right and the findings are all about seams.
|
2 participants
Issue for this PR
Closes #1155
Type of change
What does this PR do?
Stacked on #1167 — review that first; this PR is the commit on top. It is the precedence change from #1156 restacked onto the overlay attach; the mechanism is unchanged, the attach seam it reads is now the overlay's.
When a bound workspace's engine is attached, the model gets two ways to do the same thing: the native warehouse tools over local keychain connections, and the engine's MCP tools over the workspace's SaaS connection. Nothing chose between them, so the model picked whichever description read better — and that pick decided which credentials ran the query and whether it was audited (engine calls are audited server-side; native ones are not).
This adds a per-session decision: shadow only what materialised and is attributable to the bound workspace; anything undetermined runs locally and says why; nothing is silent.
attached(the overlay's own pinned engine, connected at this turn boundary) and the configured entry's pin names the bound workspace. Any other outcome, or none, fails open with a reason.--integrations=localturns it off for a session.Two deliberate deviations: the guard needs a companion call to attach the fail-open notice, which a pre-execution check cannot do; and an adjacent warehouse-type reporting bug is left alone, since fixing it changes a shipped telemetry field.
How did you verify your code works?
bun run typecheckclean; precedence, default-target, guard-order and workspace suites pass (161 tests across the six directly affected files; the tool/native/prompt suites green). The union test now asserts the allowlist is exactlyattachedover the whole outcome union, so a future outcome kind refuses routing by default.End-to-end rows from #1156 (shadow marking, redirect with a proven no-local-execution control, DuckDB control, default target, model following the redirect unprompted, write confirmation, escape hatch) are re-run on this stack and recorded in the review-log comment below before this leaves draft.
GitGuardian flags a masked placeholder (eight literal asterisks) in a help-text snapshot that only moved columns; it is present unchanged on the base commit and is not a credential.
Screenshots / recordings
n/a — CLI change, no UI.
Checklist
Summary by cubic
Previously, attaching a bound workspace's engine left the model with two equally-visible ways to run each warehouse operation — native tools over the local keychain connection and the engine's MCP tools over the SaaS connection — and nothing chose between them, so the better-written description decided which credentials ran the query and whether it was audited server-side. A per-session precedence decision now routes each operation to the engine's tool only when that tool is materialised and attach proves the engine owns the bound workspace; anything undetermined runs locally with a stated reason, and
--integrations=localdisables routing for a session. Closes #1155.Routing rules
attachedplus a configured pin naming the bound workspace; any other outcome fails open with a reason.Written for commit b8daef2. Summary will update on new commits.